Implementation of SEP-986: Specify Format for Tool Names #551
Conversation
Adds validation for MCP tool naming conventions as specified in SEP-986. Ensures Rust SDK enforces standardized tool name formats, provides clear errors for invalid names, and improves consistency across implementations. Signed-off-by: tanish111 <[email protected]>
Update doctest examples to use the correct module path rmcp::handler::server::tool_name_validation instead of rmcp::model::tool_name_validation. Signed-off-by: tanish111 <[email protected]>
github-actions
bot
added
T-core
tanish111
changed the title
alexhancock
left a comment
alexhancock
left a comment
There was a problem hiding this comment.
LGTM other than the one comment. I also prefer to remove most of the comments for code that is already clear.
Prevent empty warnings array from triggering warning output. Signed-off-by: tanish111 <[email protected]>
Signed-off-by: tanish111 <[email protected]>
Signed-off-by: tanish111 <[email protected]>
| pub fn add_route(&mut self, item: ToolRoute<S>) { | ||
| self.map.insert(item.attr.name.clone(), item); | ||
| let new_name = &item.attr.name; | ||
| // Validate tool name according to SEP specification |
There was a problem hiding this comment.
I'd remove these comments as it already seems clear based on method name
There was a problem hiding this comment.
@alexhancock I have updated add_route fn
Signed-off-by: tanish111 <[email protected]>
| } | ||
|
|
||
| /// Validates a tool name according to the SEP specification. | ||
| pub fn validate_tool_name(name: &str) -> ToolNameValidationResult { |
There was a problem hiding this comment.
these don't need to be pub right?
only validate_and_warn_tool_name
There was a problem hiding this comment.
@alexhancock I kept it pub because I wanted to keep it consistent with TS SDK
There was a problem hiding this comment.
Given that the tool name validation is only for the purpose of logging warnings right now, I think we should keep the other things private
|
|
||
| /// Result of tool name validation containing validation status and warnings. | ||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub struct ToolNameValidationResult { |
There was a problem hiding this comment.
@alexhancock should I change this to private as well as It's not used anywhere outside this file?(and all its fields and functions)
There was a problem hiding this comment.
yes. only things which are used outside the module should be private IMO. easier to expose things later if we want vs take things private.
There was a problem hiding this comment.
yes. only things which are used outside the module should be private IMO. easier to expose things later if we want vs take things private.
you mean only things which are used outside the module should be public
There was a problem hiding this comment.
yes sorry typoed. it looks good to me now.
Made ToolNameValidationResult and its functions and fields private. Made validate_tool_name and issue_tool_name_warning private. Signed-off-by: tanish111 <[email protected]>
3 participants
Motivation and Context
modelcontextprotocol/modelcontextprotocol/issues/986
How Has This Been Tested?
Manual testing in examples/servers: added name to existing #[tool] attributes in common/counter.rs with valid and invalid names. Rebuilding and running showed expected warnings. Unit tests (11) cover validation scenarios; integration tests are planned.
Breaking Changes
None, but console warnings may appear for SDK users who have names that do not satisfy the SEP.
Types of changes
Checklist
Additional context
All design decisions, names, and other aspects follow the TypeScript implementation for consistency across SDKs.
Related issue #518